Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

boards: thingy91x: fix swapped blue/green LEDs #19667

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

boards: thingy91x: fix swapped blue/green LEDs #19667

wants to merge 1 commit into from

Conversation

szczys
Copy link
Contributor

@szczys szczys commented Dec 19, 2024

The mapping for blue and green LEDs has been swapped for both leds and pwmleds nodes to correctly control the desired color.

Issue

From the LED table on the Thingy91x hardware documentation we expect the following LED mappings:
image

However we can see from the current board mapping that blue and green are reversed.

sdk-nrf/boards/nordic/thingy91x/thingy91x_nrf9151_common.dts

Lines 29 to 43 in 84ec4da

leds {
compatible = "gpio-leds";
red_led: led_1 {
gpios = <&gpio0 29 0>;
label = "RGB red channel";
};
green_led: led_2 {
gpios = <&gpio0 30 0>;
label = "RGB green channel";
};
blue_led: led_3 {
gpios = <&gpio0 31 0>;
label = "RGB blue channel";
};
};

Reproducing the issue

➜ git rev-parse HEAD
84ec4da2bfb18ecc3a6adffb4ae67cb520634e04

Verify that the blue and green LEDs are reversed by running the zephyr/samples/basic/blinky application with the following patch:

diff --git a/samples/basic/blinky/src/main.c b/samples/basic/blinky/src/main.c
index 4cab4969d94..80f491f7161 100644
--- a/samples/basic/blinky/src/main.c
+++ b/samples/basic/blinky/src/main.c
@@ -12,7 +12,7 @@
 #define SLEEP_TIME_MS   1000

 /* The devicetree node identifier for the "led0" alias. */
-#define LED0_NODE DT_ALIAS(led0)
+#define LED0_NODE DT_NODELABEL(green_led)

 /*
  * A build error on this line means your board is unsupported.

Correcting pwmleds

I could not find any documentation on the hardware page or in the available schematic for how the PWM channels should be mapped. However, when comparing the Thingy91 to the Thingy91x, pwmleds for green and blue are swapped in the same way described above. This PR also corrects this.

@szczys
boards: thingy91x: fix swapped blue/green LEDs
fb5c71f 
The mapping for blue and green LEDs has been swapped for both `led` and
`pwm_led` nodes to correctly control the desired color.

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys requested review from a team as code owners December 19, 2024 20:32
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Dec 19, 2024
@NordicBuilder
Copy link
Contributor

Thank you for your contribution!
It seems you are not a member of the nrfconnect GitHub organization. External contributions are handled as follows:
Large contributions, affecting multiple subsystems for example, may be rejected if they are complex, may introduce regressions due to lack of test coverage, or if they are not consistent with the architecture of nRF Connect SDK.
PRs will be run in our continuous integration (CI) test system.
If CI passes, PRs will be tagged for review and merged on successful completion of review. You may be asked to make some modifications to your contribution during review.
If CI fails, PRs may be rejected or may be tagged for review and rework.
PRs that become outdated due to other changes in the repository may be rejected or rework requested.
External contributions will be prioritized for review based on the relevance to current development efforts in nRF Connect SDK. Bug fix PRs will be prioritized.
You may raise issues or ask for help from our Technical Support team by visiting https://devzone.nordicsemi.com/.

Note: This comment is automatically posted and updated by the Contribs GitHub Action.

@NordicBuilder NordicBuilder added the external External contribution label Dec 19, 2024
@nordicjm nordicjm requested a review from maxd-nordic December 20, 2024 08:37
@jtguggedal jtguggedal added the CI-Requested Approves single commit for CI tests on Internal HW label Jan 6, 2025
jtguggedal
jtguggedal approved these changes Jan 6, 2025
View reviewed changes
Copy link
Contributor

@jtguggedal jtguggedal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching and fixing this issue!

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 6, 2025 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 1

Inputs:

Sources:

sdk-nrf: PR head: fb5c71f08264fc545ec417125328c96973a05f59

more details

sdk-nrf:

PR head: fb5c71f08264fc545ec417125328c96973a05f59
merge base: 84ec4da2bfb18ecc3a6adffb4ae67cb520634e04
target head (main): 5b86006fd15bf18b90881b2465c4196f2000e4a9
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (1) 
boards
│  ├── nordic
│  │  ├── thingy91x
│  │  │  │ thingy91x_nrf9151_common.dts

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 274
  • ✅ Integration tests
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-chip
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-nrf_crypto
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-tfm
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-find-my
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-sidewalk
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simensrostad simensrostad simensrostad approved these changes

@jtguggedal jtguggedal jtguggedal approved these changes

@maxd-nordic maxd-nordic Awaiting requested review from maxd-nordic

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. CI-Requested Approves single commit for CI tests on Internal HW external External contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@szczys @NordicBuilder @jtguggedal @simensrostad